[stable33] fix: Disable imagick preview providers - #62148
Merged
Conversation
Signed-off-by: Joas Schilling <coding@schilljs.com>
nickvergessen
requested review from
Altahrim,
ArtificialOwl,
come-nc and
salmart-dev
and removed request for
a team
July 15, 2026 04:47
provokateurin
approved these changes
Jul 15, 2026
ChristophWurst
approved these changes
Jul 15, 2026
Merged
helix-nine
added a commit
to Start9Labs/nextcloud-startos
that referenced
this pull request
Jul 29, 2026
Latest release on the 33 line, cut 2026-07-23. Maintenance only: no shipped-app changes (`core/shipped.json` is identical between v33.0.6 and v33.0.7), no PHP floor change (`lib/versioncheck.php` is byte-identical), so the preserved-defaults list in `disableUnstableApps` is unaffected. Carries security hardening (sabre/xml callable deserialization disabled, federated-share rate limiting, lost-password form throttling, user-search disclosure tightening, code-signing revocation list and CA bundle refresh), the usual sharing/DAV/external-storage fixes, and perf work on PROPFIND, trashbin deletes and the photocache. It also carries one user-visible regression, so the release notes call it out: upstream disabled the ImageMagick preview providers as a security stopgap (nextcloud/server#62148 on stable33), and `PreviewManager` gates the whole imagick block on that, so PDF, SVG, TIFF, HEIC, PSD, EPS, TTF, TGA and SGI previews are gone until 33.0.8. The re-enable merged to stable33 on 2026-07-29 as #62619 but is not tagged yet. The revision resets to :0 because the upstream version changed. This is now a real application upgrade rather than a StartOS-only revision bump, so `runUpstreamUpgrade` no longer early-returns and `occ upgrade` runs during init — on a 0.3.5x instance that happens after the layout migration in the same update. 32 -> 33 is a single major step, so the one-major-at-a-time guard in `bootstrapNextcloud` still permits it.
dr-bonez
pushed a commit
to Start9Labs/nextcloud-startos
that referenced
this pull request
Jul 29, 2026
…; bump Nextcloud to 33.0.7 (33.0.7:0) (#121) * fix: verify the cluster before treating the 0.3.5x relocation as done; 33.0.6:2 → 33.0.6:3 The Postgres entrypoint runs `mkdir -p "$PGDATA"` in docker_create_db_directories *before* docker_verify_minimum_env decides the database is uninitialized and exits 1. So a Postgres start against a volume whose cluster has not yet been relocated leaves an empty `data/` behind — and both setupMain and the init upgrade start Postgres. Both guards in this migration tested for that directory rather than for a database: - relocatePostgres: `test -d $PGDATA` -> skip the move - isNeverStarted: stat() on the three candidate paths An empty `data/` therefore reads as "already relocated". The migration skips the move, runs the config import and the permissions walk, and deletes start9/config.yaml on its way out — the marker gating the whole block. From that point it is disarmed: it completes in ~3 seconds doing nothing, the cluster is never moved, and every Postgres start finds an empty PGDATA, recreates it and exits 1. Fixes: - findCluster() locates the cluster by PG_VERSION across data, 17/main and 15/main, and returns null when there isn't one. Replaces both directory-existence guards. - The migration throws before touching anything when no cluster is found, so it cannot delete the marker on a run that would migrate nothing. The two failure cases are now distinguishable: an instance with app data is told to restore from a backup and explicitly not to uninstall; only a genuinely never-started instance gets the uninstall-and-reinstall advice, which for a populated instance was wrong. - The move rmdir's the empty PGDATA shell first. rmdir, not rm -rf: it refuses a non-empty directory, so an unexpected state fails loudly rather than being deleted. Without it, `mv src $PGDATA` moves the source *inside* the existing directory (data/docker) rather than into place. - Same PG_VERSION test and rmdir applied to the 0.4.0-beta relocation (17/docker -> data), which carried the identical latent bug. Also reports progress during the permissions walk. The SDK hands every migration a FullProgressTracker for exactly this and this one destructured only `effects`. The walk corrects permissions on every file under data/ one directory at a time (a single recursive chmod OOMs in the migration subcontainer), so its cost scales with file count and on a large instance it runs for hours with the update UI showing a bar that never moves. Now reports a running count of directories processed, on the same 100-directory cadence as the log line. No total: establishing one costs a second full metadata walk, about as expensive as the work itself. And untangles the naming, which made the above hard to reason about — both run during init and the names were near-synonyms for different systems: migrateNextcloud -> repairPermissionsFrom035x (chmods, doesn't migrate) relocatePostgres -> relocatePostgresFrom035x migrateConfig -> importConfigFrom035x upgradeNextcloud -> runUpstreamUpgrade The 0.3.5x body moves out of current.ts into versions/from035x.ts, leaving current.ts as version + release notes + a one-line call (388 -> 106 lines). Each file's header names the other and states the ordering: the StartOS layout migration is driven by the version graph, the upstream app upgrade by the bundled Nextcloud release, and versionGraph precedes bootstrapNextcloud in sdk.setupInit. * fix: clear a stale postmaster.pid before starting postgres Hardening. No confirmed occurrence — kept because the mechanism is real, and in its own commit so it can be dropped independently of the preceding one. PostgreSQL writes postmaster.pid into PGDATA while running and removes it on a clean exit. SIGTERM to postgres is a *smart* shutdown — it waits for clients to disconnect — and the SDK escalates to SIGKILL after 60s, so a stop that outruns that budget leaves the file behind. On the next start postgres reads the PID it names and aborts with FATAL: lock file "postmaster.pid" already exists if that PID is alive. Each chain build runs in a fresh PID namespace with a fresh, low PID assignment, so the recorded PID can be live and owned by an unrelated process. getBaseDaemons is the only place in the stack that starts postgres without this prelude. The SDK's Backups.withPgDump does it before every pg_ctl start (lib/backup/Backups.ts), and the 0.3.5x relocation does it too — the daemon path was simply missed. Removal is unconditional, which is safe because nothing else can hold the data directory: backups only run once the service is stopped, both init paths run with the service stopped, and the chain reconciler fully terms an entry before starting its replacement. Runs as root so ownership can never block the unlink and wedge the chain on the new oneshot. Placing it in getBaseDaemons covers setupMain, install init and update init in one place. It does not re-run if postgres crash-restarts within a single container lifetime, but that case self-heals — same PID namespace, so postgres correctly identifies its own stale file. * fix: refuse the 0.3.5x relocation unless the cluster is a completed PG 17 `findCluster` accepted a location on the mere existence of `PG_VERSION`, and `PG_LOCATIONS` includes `15/main`. 0.3.5x ran its own `pg_upgrade` 15 -> 17 in copy mode and reaped `15/main` on the *next* launch, gated on `.pg17_upgrade_complete`, so a surviving `15/main` means one of two things: - marker present: the upgrade finished, `17/main` is authoritative and `15/main` is a fallback that was never reaped. Migrating is correct. - marker absent: the upgrade never finished. `15/main` holds the real data and any `17/main` beside it is the empty initdb stub — which `findCluster` prefers, since `17/main` sorts first. Both were relocated regardless, completing the migration and deleting the 0.3.5x marker over a cluster PostgreSQL 17 cannot open. Measured on a 0.4.0 VM: `15/main` alone crash-looped `database files are incompatible with server` with the migration gone. master failed safe here — its `mv 17/main` threw — so this was a regression. Now `clusterVersion` reads `PG_VERSION` rather than stat()ing it, the marker decides whether the 15 -> 17 upgrade completed, and a non-17 cluster is refused as a backstop. The beta relocation uses the same read, so its comment claiming parity with `findCluster` is true again. `rmdir $PGDATA` used non-throwing `exec` with the result discarded, so the comment promising it "fails loudly rather than deleting a database" described a branch that did not exist: with a non-empty `data/`, `rmdir` failed silently and `mv` nested the cluster at `data/main`, reporting success. `execFail` alone is wrong — on the happy path `$PGDATA` is absent and `rmdir` legitimately fails — so `clearPgdataShell` checks the postcondition instead. Also: the file header claimed the migration "runs at most once, on the first update away from 32.0.11:0", but the auto-generated `< current` vertex runs `up` on every update into current; the i18n comment for key 134 pointed at `versions/current.ts` after this branch moved the call site; and AGENTS.md named subcontainers that do not exist (`nextcloud`, `cron`, `postgres` for `nextcloud-sub`, `nextcloud-cron`, `postgres-sub`). * chore: bump Nextcloud to 33.0.7; 33.0.6:3 -> 33.0.7:0 Latest release on the 33 line, cut 2026-07-23. Maintenance only: no shipped-app changes (`core/shipped.json` is identical between v33.0.6 and v33.0.7), no PHP floor change (`lib/versioncheck.php` is byte-identical), so the preserved-defaults list in `disableUnstableApps` is unaffected. Carries security hardening (sabre/xml callable deserialization disabled, federated-share rate limiting, lost-password form throttling, user-search disclosure tightening, code-signing revocation list and CA bundle refresh), the usual sharing/DAV/external-storage fixes, and perf work on PROPFIND, trashbin deletes and the photocache. It also carries one user-visible regression, so the release notes call it out: upstream disabled the ImageMagick preview providers as a security stopgap (nextcloud/server#62148 on stable33), and `PreviewManager` gates the whole imagick block on that, so PDF, SVG, TIFF, HEIC, PSD, EPS, TTF, TGA and SGI previews are gone until 33.0.8. The re-enable merged to stable33 on 2026-07-29 as #62619 but is not tagged yet. The revision resets to :0 because the upstream version changed. This is now a real application upgrade rather than a StartOS-only revision bump, so `runUpstreamUpgrade` no longer early-returns and `occ upgrade` runs during init — on a 0.3.5x instance that happens after the layout migration in the same update. 32 -> 33 is a single major step, so the one-major-at-a-time guard in `bootstrapNextcloud` still permits it. --------- Co-authored-by: Matt Hill <9935159+MattDHill@users.noreply.github.com> Co-authored-by: Helix <267227783+helix-nine@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #62145